TASK-6565 Ensure the CellBase DB adaptors are updated to reflect the latest data releases #700
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
On branch TASK-6565 Changes to be committed: new file: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/DataReleaseSingleton.java new file: cellbase-lib/src/test/java/org/opencb/cellbase/lib/impl/core/DataReleaseSingletonTest.java
…tants, #TASK-6565 On branch TASK-6565 Changes to be committed: modified: cellbase-app/src/main/java/org/opencb/cellbase/app/cli/admin/executors/LoadCommandExecutor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/EtlCommons.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/CellBaseDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/ClinicalMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/GeneMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/GenomeMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/MetaMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/MissenseVariationFunctionalScoreMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/OntologyMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/PharmacogenomicsMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/ProteinMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/PublicationMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/RegulationMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/RepeatsMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/SnpMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/SpliceScoreMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/TranscriptMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/VariantMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/XRefMongoDBAdaptor.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/managers/CellBaseManagerFactory.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/managers/DataReleaseManager.java modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/variant/annotation/VariantAnnotationCalculator.java
On branch TASK-6565 Changes to be committed: modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/DataReleaseSingleton.java modified: cellbase-server/src/main/java/org/opencb/cellbase/server/rest/GenericRestWSServer.java
On branch TASK-6565 Changes to be committed: modified: cellbase-server/src/main/java/org/opencb/cellbase/server/rest/GenericRestWSServer.java
On branch TASK-6565 Changes to be committed: modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/DataReleaseSingleton.java
On branch TASK-6565 Changes to be committed: modified: cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/DataReleaseSingleton.java
jtarraga
changed the title
j-coll
requested changes
Aug 7, 2024
cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/DataReleaseSingleton.java
Outdated
Show resolved
Hide resolved
cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/DataReleaseSingleton.java
Outdated
Show resolved
Hide resolved
| try { | ||
| handleDocumentChange(changeStreamDocument); | ||
| } catch (CellBaseException e) { | ||
| LOGGER.warn("Exception from handle document change function: {}", e.getStackTrace()); |
There was a problem hiding this comment.
Improper exception reporting. The whole exception should be the last element of the logger.
LOGGER.warn("Exception from handle document change function", e);
Also, might want to add more details on the collection watcher that failed.
There was a problem hiding this comment.
And what if the whole collection.watch()... fails? The whole code of the thread should be within a pseudo infinite loop, catching any Exception, and check for thread interruptions within the loop
while (!Thread.currentThread().isInterrupted()) {
try {
MongoCursor iterator = collection.watch().fullDocument(FullDocument.UPDATE_LOOKUP).iterator();
while (!Thread.currentThread().isInterrupted() && iterator.hasNext()) {
// ...
}
} catch (Exception e) {
LOGGER.warn("...", e);
}
}There was a problem hiding this comment.
Improper exception reporting. The whole exception should be the last element of the logger.
LOGGER.warn("Exception from handle document change function", e);Also, might want to add more details on the collection watcher that failed.
maybe it's better to display the strack trace, e.g.:
LOGGER.warn("Exception from handle document change function: {}", Arrays.toString(e.getStackTrace()));
There was a problem hiding this comment.
Having the exception as the last argument in a logger will print the whole stacktrace
try {
throw new RuntimeException("myException");
} catch (Exception e) {
logger.warn("This is an exception", e);
}2024-08-08 12:03:27 [main] WARN VariantQueryExecutorTest:59 - This is an exception
java.lang.RuntimeException: myException
at org.opencb.opencga.storage.core.variant.query.executors.VariantQueryExecutorTest.setUp(VariantQueryExecutorTest.java:57) ~[test-classes/:?]
at org.opencb.opencga.storage.hadoop.variant.query.executors.HadoopVariantQueryExecutorTest.setUp(HadoopVariantQueryExecutorTest.java:25) ~[test-classes/:?]
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_161]
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:1.8.0_161]
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:1.8.0_161]
at java.lang.reflect.Method.invoke(Method.java:498) ~[?:1.8.0_161]
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59) ~[junit-4.13.2.jar:4.13.2]
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) ~[junit-4.13.2.jar:4.13.2]
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56) ~[junit-4.13.2.jar:4.13.2]
at org.junit.internal.runners.statements.RunBefores.invokeMethod(RunBefores.java:33) ~[junit-4.13.2.jar:4.13.2]
.....
| LOGGER.warn("Exception from handle document change function: {}", e.getStackTrace()); | ||
| } | ||
| }); | ||
| }).start(); |
There was a problem hiding this comment.
All these threads should be within a single "ExecutionService". Then, as this is a singleton class, and can't be closed easily, a ShutdownHook should terminate the ExecutionService
Runtime.getRuntime().addShutdownHook(new Thread(() -> {
threadPool.shutdownNow();
});
j-coll
reviewed
Aug 8, 2024
| DatabaseInfo dbInfo = getDatabaseInfo(dbname); | ||
|
|
||
| // Lock and load data if necessary | ||
| dbInfo.getRwLock().writeLock().lock(); |
There was a problem hiding this comment.
This would be an unneeded lock if dbInfo.getCacheData().containsKey(release) and dbInfo.getCacheData().get(release).containsKey(data). Given that this method will be called a lot of times, might be worth improving the logic?
j-coll
reviewed
Aug 8, 2024
| private String dbName; | ||
| private String species; | ||
| private String assembly; | ||
| private ReentrantReadWriteLock rwLock; |
There was a problem hiding this comment.
Do you actually need a ReentrantReadWriteLock? Why not using a ReentrantLock?
j-coll
reviewed
Aug 8, 2024
| @@ -85,13 +79,31 @@ private DataReleaseSingleton(CellBaseManagerFactory managerFactory) throws CellB | |||
| .getCollectionName()); | |||
| // Set up the change stream for the collection | |||
| new Thread(() -> { | |||
| collection.watch().fullDocument(FullDocument.UPDATE_LOOKUP).forEach(changeStreamDocument -> { | |||
| while (true) { | |||
There was a problem hiding this comment.
I'd still have here a !Thread.currentThread().isInterrupted()
j-coll
previously approved these changes
Sep 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TASK-6565